core(metrics): consumable metrics audit output #5101
Conversation
patrickhulce
force-pushed
the
better_metrics_js
branch
from
37e9f08
to
763ac8b
Compare
|
@denar90 @pedro93 @benschwarz @alekseykulikov hows this look to you? you'll get all metrics within this audit instead of relying on |
lighthouse-core/audits/metrics.js
Outdated
| for (const [name, value] of Object.entries(metrics)) { | ||
| const key = /** @type {keyof UberMetricsItem} */ (name); | ||
| if (typeof value === 'undefined') { | ||
| delete metrics[key]; |
There was a problem hiding this comment.
maybe copy defined values to a new metrics object to return rather than delete from the original?
There was a problem hiding this comment.
any particular reason for this? I'll end up just creating an empty object with no type and casting at the end which seems sad 😢
There was a problem hiding this comment.
I guess we can do it for now. delete is just almost always a bad idea :(
On the compiler side, this isn't really type checked either, since key has to be cast and value ends up as any. We just know that undefined and a number that can be rounded are the two possible cases. I don't know a great solution, though. We'd need an Object.entries() that doesn't widen, an Array.filter() that really filters types, and an Object.fromEntries() from TC39 :)
The other option is to leave them defined as undefined. Was that bad? They'll get dropped from the JSON and we're testing against undefined anyways
There was a problem hiding this comment.
yeah I guess it's not the end of the world to leave them undefined, will do
| const values = metricsAudit.details.items.find(item => item.metricName === metricName); | ||
| return values && values[timingOrTimestamp]; | ||
| const values = metricsAudit.details.items[0]; | ||
| return values && values[metricName]; |
There was a problem hiding this comment.
this can be a TODO, but seems like we can just get rid of this whole findValueInMetricsAuditFn thing now. If there's no metrics audit, pwmetrics can't really do much, so it could check once at the beginning. Then all the gets are just property dereferencing with the new object layout.
(metricsDefinitions might still be useful, haven't looked into it)
There was a problem hiding this comment.
sure TODO added
|
last 📞 , any objections? |
lighthouse-core/audits/metrics.js
Outdated
| for (const [name, value] of Object.entries(metrics)) { | ||
| const key = /** @type {keyof UberMetricsItem} */ (name); | ||
| if (typeof value === 'undefined') { | ||
| delete metrics[key]; |
There was a problem hiding this comment.
I guess we can do it for now. delete is just almost always a bad idea :(
On the compiler side, this isn't really type checked either, since key has to be cast and value ends up as any. We just know that undefined and a number that can be rounded are the two possible cases. I don't know a great solution, though. We'd need an Object.entries() that doesn't widen, an Array.filter() that really filters types, and an Object.fromEntries() from TC39 :)
The other option is to leave them defined as undefined. Was that bad? They'll get dropped from the JSON and we're testing against undefined anyways
|
Looks nice, great work guys 👍 |
after much ado about nothing and the repeated creation/deletion of summary, we're just making a single item inside details that has all the metrics keyed by name and
Ts. @paulirish approved of this approachit means that querying will look like
audits.metrics.details.items[0].firstContentfulPaintwe'll just commit to not breaking that contract with uber typechecking 🎉
ref #5008